Hide experimental properties from the JSON source generator to avoid MCPEXP001 diagnostics#1260
Hide experimental properties from the JSON source generator to avoid MCPEXP001 diagnostics#1260MackinnonBuck wants to merge 1 commit intomainfrom
Conversation
…MCPEXP001 diagnostics
| [JsonPropertyName("task")] | ||
| public McpTaskMetadata? Task { get; set; } | ||
| [JsonConverter(typeof(ExperimentalJsonConverter<McpTaskMetadata>))] | ||
| internal object? _task; |
There was a problem hiding this comment.
Today, JsonInclude against inaccessible members only works with reflection but not the source generator. We'd want to mark this field as public and additionally EditorBrowsable.Never. Because McpTaskMetadata is itself marked as experimental this will likely cause experimental warnings to resurface on account of it producing new ExperimentalJsonConverter<McpTaskMetadata>() expressions.
There was a problem hiding this comment.
Thanks for pointing this out. This actually made me realize that the backing field pattern may introduce binary breaking changes regardless of how we structure it. When the experimental API stabilizes, removing the backing field and its converter breaks consumer source-generated code that references them, even for consumers who never used the experimental feature. The only way to avoid that is to keep the backing field and converter around indefinitely, which isn't ideal.
I'm starting to think a new feature in System.Text.Json might be the best way to solve this. One option is to introduce a new attribute like [JsonSourceGenerationIgnoreByDefault] that can be applied to a type. The source generator would skip generating serialization logic for that type unless the consumer explicitly registers it via [JsonSerializable(typeof(T))] on their JsonSerializerContext. We'd apply it to all experimental DTO types (e.g., McpTaskMetadata, McpTasksCapability). Consumers who don't use Tasks get no diagnostics and no generated code for those types. Consumers who do use Tasks can opt-in explicitly by adding the [JsonSerializable] entry, accepting the instability risk. When the type stabilizes, we remove the attribute, and the type becomes source-gen-visible by default, which is additive rather than breaking. The JsonContext in McpJsonUtilities would explicitly opt-in experimental types for serialization so that everything "just works" when using the SDK-provided JsonSerializerOptions. Does something like this seem feasible, @eiriktsarpalis? The main drawback I see is that it might not be obvious to developers why experimental properties aren't getting serialized by default when using a custom JsonSerializerContext.
There was a problem hiding this comment.
When it comes to improving the STJ generator itself, we have many options. The problem with that approach of course is that it won't be available to this library before the next LTS version of .NET gets released.
|
|
||
| namespace ModelContextProtocol.Tests; | ||
|
|
||
| public static class ExperimentalJsonConverterTests |
There was a problem hiding this comment.
ModelContextProtocol.Tests currently suppresses experimental warnings. Do we need any testing checking that applying the source generator on MCP types doesn't trigger experimental warnings? I foresee that this could easily regress in the future without us noticing.
2 participants
Summary
Prevent experimental API types (Tasks, ToolExecution) from leaking into user code via the System.Text.Json source generator, eliminating spurious MCPEXP001 diagnostics for users who don't use experimental features.
Description
When users define their own
JsonSerializerContextthat includes MCP protocol types (e.g.,Tool,ServerCapabilities), the System.Text.Json source generator follows property types transitively and generates code that references experimental types. This causes MCPEXP001 errors in user projects even when they aren't using experimental features like Tasks.Approach: Object Backing Field Pattern
For each stable type that exposes an experimental property, the public typed property is now marked with
[JsonIgnore]and backed by aninternal object?field annotated with[JsonInclude],[JsonPropertyName], and a new[JsonConverter(typeof(ExperimentalJsonConverter<T>))]. Because the source generator seesobject?rather than the experimental type, it no longer walks the experimental type graph, so no MCPEXP001 diagnostic is emitted in consuming projects.The
ExperimentalJsonConverter<T>delegates serialization toMcpJsonUtilities.DefaultOptions, which already contains source-generated contracts for all experimental types. This preserves NativeAOT compatibility and ensures the experimental properties continue to round-trip correctly over the wire.A
DiagnosticSuppressorwas considered but is not necessary since the backing field pattern fully hides experimental types from the source generator (and it wouldn't help anyway becauseDiagnosticSuppressors cannot suppress error-severity diagnostics).Other changes
[Experimental(MCPEXP001)]to the 6 experimental properties that were missing it (onlyTool.Executionhad it previously). Happy to undo this change if the omission of the[Experimental]attribute was deliberate.WriteIndentedpropagation.Fixes #1255